fix: check daily if update is due based on UPDATE_INTERVAL - #303
fix: check daily if update is due based on UPDATE_INTERVAL#303aaronspruit wants to merge 13 commits into
Conversation
aaronspruit
changed the title
|
Thanks for the PR! This approach is something I have considered initially. Having the schedule reset on container restart is really not ideal. However, there are some issues with this:
I'm happy to hear your thoughts on this as I haven't found the |
|
Ah, ok, I see what you're trying to avoid. It just wasn't clear from the ENV VAR what was going on - and that the container had to be UP for the complete
Agreed. If you have the
Also agreed. I guess the issue becomes do you want people to artificially lower the Another mitigation would be around date-versioning the hosted tar and doing a lookup. I have no idea how often the dataset is actually updated on your mirror either, and so I don't know if I change Just some ideas. |
|
Mine hadn't updated since February due to this issue. Interested in seeing a fix for this, whether it's this change, a minor change to this approach, or a new approach. Willing to help also. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe process manager now polls every 60 seconds and checks index age before updates. Update attempts use a one-hour backoff. Interval parsing and index-age handling are covered by tests, and the README documents the new behaviour. ChangesIndex-age update polling
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e3ba7a1-1f9e-4433-820e-ca87954d2698
📒 Files selected for processing (2)
src/process_manager.pytests/test_process_manager.py
| def _parse_interval(self, interval: str) -> datetime.timedelta: | ||
| interval = interval.lower() | ||
| value = int(interval[:-1]) | ||
| unit = interval[-1] | ||
| if unit == "d": | ||
| return datetime.timedelta(days=value) | ||
| if unit == "h": | ||
| return datetime.timedelta(hours=value) | ||
| if unit == "m": | ||
| return datetime.timedelta(minutes=value) | ||
| logger.warning(f"Invalid UPDATE_INTERVAL format: {interval}, defaulting to 1 day") | ||
| return datetime.timedelta(days=1) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle malformed interval values before conversion.
int(interval[:-1]) raises for values such as "", "d", and "invalid". The fallback at the end of this method is then unreachable. The scheduled job will fail instead of using the stated one-day default.
Catch ValueError and IndexError before reading the unit. Add malformed-value cases to test_parse_interval.
Proposed fix
def _parse_interval(self, interval: str) -> datetime.timedelta:
interval = interval.lower()
- value = int(interval[:-1])
- unit = interval[-1]
+ try:
+ value = int(interval[:-1])
+ unit = interval[-1]
+ except (ValueError, IndexError):
+ logger.warning(f"Invalid UPDATE_INTERVAL format: {interval}, defaulting to 1 day")
+ return datetime.timedelta(days=1)
if unit == "d":
return datetime.timedelta(days=value)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _parse_interval(self, interval: str) -> datetime.timedelta: | |
| interval = interval.lower() | |
| value = int(interval[:-1]) | |
| unit = interval[-1] | |
| if unit == "d": | |
| return datetime.timedelta(days=value) | |
| if unit == "h": | |
| return datetime.timedelta(hours=value) | |
| if unit == "m": | |
| return datetime.timedelta(minutes=value) | |
| logger.warning(f"Invalid UPDATE_INTERVAL format: {interval}, defaulting to 1 day") | |
| return datetime.timedelta(days=1) | |
| def _parse_interval(self, interval: str) -> datetime.timedelta: | |
| interval = interval.lower() | |
| try: | |
| value = int(interval[:-1]) | |
| unit = interval[-1] | |
| except (ValueError, IndexError): | |
| logger.warning(f"Invalid UPDATE_INTERVAL format: {interval}, defaulting to 1 day") | |
| return datetime.timedelta(days=1) | |
| if unit == "d": | |
| return datetime.timedelta(days=value) | |
| if unit == "h": | |
| return datetime.timedelta(hours=value) | |
| if unit == "m": | |
| return datetime.timedelta(minutes=value) | |
| logger.warning(f"Invalid UPDATE_INTERVAL format: {interval}, defaulting to 1 day") | |
| return datetime.timedelta(days=1) |
|
I apologize for my late response to this. Let me know if you're happy with me merging this or if you would like to follow up on this. Thanks for the patience and the contribution! |
3 participants
Originally updates would only be checked if they need to happen based on the duration of UPDATE_INTERVAL. This means that if the container is restarted, it resets the timer.
This change compares the timestamp on DATA_DIR/.photon-index-updated to the UPDATE_INTERVAL on a daily basis.
I believe this was the originally intended functionality, as by default it means the container needs to be running for 30 days before an update is even attempted...instead of doing updates every UPDATE_INTERVAL.
I was very confused as to why my service hadn't updated when the last time it did so was Feb 17th and UPDATE_INTERVAL=30d (even with bouncing the container). This fix correctly identified that today it's 45 days out of date and did the update.